-
Notifications
You must be signed in to change notification settings - Fork 5.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Automation] Refactoring Automation Resources #3190
[Automation] Refactoring Automation Resources #3190
Conversation
Automation for azure-libraries-for-javaNothing to generate for azure-libraries-for-java |
Automation for azure-sdk-for-pythonThe initial PR has been merged into your service PR: |
Automation for azure-sdk-for-nodeThe initial PR has been merged into your service PR: |
Automation for azure-sdk-for-goEncountered a Subprocess error: (azure-sdk-for-go)
Command: ['/usr/local/bin/autorest', '/tmp/tmp24yyqvao/rest/specification/automation/resource-manager/readme.md', '--go', '--go-sdk-folder=/tmp/tmp24yyqvao/src/github.com/Azure/azure-sdk-for-go', '--multiapi', '--use=@microsoft.azure/autorest.go@~2.1.100', '--use-onever', '--verbose'] AutoRest code generation utility [version: 2.0.4280; node: v7.10.1]
(C) 2018 Microsoft Corporation.
https://aka.ms/autorest
Loading AutoRest core '/root/.autorest/@microsoft.azure_autorest-core@2.0.4280/node_modules/@microsoft.azure/autorest-core/dist' (2.0.4280)
Loading AutoRest extension '@microsoft.azure/autorest.go' (~2.1.100->2.1.104)
Loading AutoRest extension '@microsoft.azure/autorest.modeler' (2.3.38->2.3.38)
Processing batch task - {"tag":"package-2015-10"} .
ERROR (Fatal/DuplicateModelCollsion): Duplicated model name with non-identical definitions
- file:///tmp/tmp24yyqvao/rest/specification/automation/resource-manager/Microsoft.Automation/stable/2015-10-31/runbook.json:1056:4 ($.definitions.JobStreamProperties)
- file:///tmp/tmp24yyqvao/rest/specification/automation/resource-manager/Microsoft.Automation/stable/2015-10-31/job.json:780:4 ($.definitions.JobStreamProperties)
- file:///tmp/tmp24yyqvao/rest/specification/automation/resource-manager/Microsoft.Automation/stable/2015-10-31/dscCompilationJob.json:561:4 ($.definitions.JobStreamProperties)
Process() cancelled due to exception : Cancellation requested.
Failure during batch task - {"tag":"package-2015-10"} -- Cancellation requested..
Cancellation requested. |
Automation for azure-sdk-for-rubyEncountered a Subprocess error: (azure-sdk-for-ruby)
Command: ['/usr/local/bin/autorest', '/tmp/tmp86k9cmzb/rest/specification/automation/resource-manager/readme.md', '--multiapi', '--ruby', '--ruby-sdks-folder=/tmp/tmp86k9cmzb/sdk', '--use=@microsoft.azure/autorest.ruby@3.0.20', '--version=preview'] AutoRest code generation utility [version: 2.0.4280; node: v7.10.1]
(C) 2018 Microsoft Corporation.
https://aka.ms/autorest
Loading AutoRest core '/root/.autorest/@microsoft.azure_autorest-core@2.0.4280/node_modules/@microsoft.azure/autorest-core/dist' (2.0.4280)
Loading AutoRest extension '@microsoft.azure/autorest.ruby' (3.0.20->3.0.20)
Loading AutoRest extension '@microsoft.azure/autorest.modeler' (2.1.22->2.1.22)
Processing batch task - {"tag":"package-2015-10"} .
ERROR (Fatal/DuplicateModelCollsion): Duplicated model name with non-identical definitions
- file:///tmp/tmp86k9cmzb/rest/specification/automation/resource-manager/Microsoft.Automation/stable/2015-10-31/runbook.json:1056:4 ($.definitions.JobStreamProperties)
- file:///tmp/tmp86k9cmzb/rest/specification/automation/resource-manager/Microsoft.Automation/stable/2015-10-31/job.json:780:4 ($.definitions.JobStreamProperties)
- file:///tmp/tmp86k9cmzb/rest/specification/automation/resource-manager/Microsoft.Automation/stable/2015-10-31/dscCompilationJob.json:561:4 ($.definitions.JobStreamProperties)
Process() cancelled due to exception : Cancellation requested.
Failure during batch task - {"tag":"package-2015-10"} -- Cancellation requested..
Cancellation requested. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix the semantic errors since they are preventing further validations to be run.
}, | ||
"description": "The response model for the list job operation." | ||
}, | ||
"DscConfigurationAssociationProperty": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this property invalidates https://github.com/Azure/azure-rest-api-specs/pull/3190/files#diff-09777627978972db514896bb1119bc44R269
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
"$ref": "../../common/v1/definitions.json#/parameters/ResourceGroupNameParameter" | ||
}, | ||
{ | ||
"$ref": "../../common/v1/definitions.json#/parameters/AutomationAccountNameParameter" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no automationAccountName needed here.
}, | ||
"allOf": [ | ||
{ | ||
"$ref": "../../common/v1/definitions.json#/definitions/ProxyResource" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DeveloperTommy Added Schedule as a ProxyResource.
} | ||
], | ||
"responses": { | ||
"201": { | ||
"200": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DeveloperTommy Changed the 201 to 200.
@@ -18,7 +18,7 @@ | |||
} | |||
}, | |||
"responses": { | |||
"201": { | |||
"200": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DeveloperTommy Changed 201 to 200.
@jianghaolu Could you please take a look at this PR. Thanks. |
} | ||
}, | ||
"201": { | ||
"description": "Software update configuration is created.", | ||
"schema": { | ||
"$ref": "./definitions.json#/definitions/softwareUpdateConfiguration" | ||
"$ref": "#/definitions/softwareUpdateConfiguration" | ||
} | ||
}, | ||
"default": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
400, 404, and 409 are not defined here so please remove them from https://github.com/vrdmr/azure-rest-api-specs/blob/0489a8a2c0bf758e9593a7d21ff507e0899ff698/specification/automation/resource-manager/Microsoft.Automation/preview/2017-05-15-preview/examples/softwareUpdateConfiguration/createSoftwareUpdateConfiguration.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
@@ -214,7 +214,7 @@ | |||
"default": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
404 is not defined here but exists in the example: https://github.com/vrdmr/azure-rest-api-specs/blob/0489a8a2c0bf758e9593a7d21ff507e0899ff698/specification/automation/resource-manager/Microsoft.Automation/preview/2017-05-15-preview/examples/softwareUpdateConfiguration/deleteSoftwareUpdateConfiguration.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
}, | ||
"Key": { | ||
"properties": { | ||
"keyName": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are lower case properties but they are upper case in the example: https://github.com/vrdmr/azure-rest-api-specs/blob/0489a8a2c0bf758e9593a7d21ff507e0899ff698/specification/automation/resource-manager/Microsoft.Automation/stable/2015-10-31/examples/listAutomationAccountKeys.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jianghaolu - Our service returns the properties in Uppercase, and if I define the property in upper case, I get the following error -
ERROR (DefinitionsPropertiesNamesCamelCase/R3016/RPCViolation): Property named: 'KeyName', for definition: 'Key' must follow camelCase style. Example: 'keyName'.
Is it a blocking issue or something which can leave it as it is in this API version and we can fix in the newer API version? Please let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That error is simply a style error - but if your swagger is not corresponding to the actual service then developers won't be able to use the clients generated from this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might want to follow the suppression process to suppress this R3016 error: https://github.com/Azure/adx-documentation-pr/wiki/Swagger-Validation-Errors-Suppression
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
"required": true, | ||
"type": "string", | ||
"description": "The automation account name." | ||
"$ref": "../../common/v1/definitions.json#/parameters/AutomationAccountNameParameter" | ||
}, | ||
{ | ||
"name": "credentialName", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
@@ -350,7 +334,7 @@ | |||
"description": "Name of the node configuration." | |||
}, | |||
"configuration": { | |||
"$ref": "./definitions.json#/definitions/DscConfigurationAssociationProperty", | |||
"$ref": "#/definitions/DscConfigurationAssociationProperty", | |||
"description": "Gets or sets the configuration of the node." | |||
}, | |||
"incrementNodeConfigurationBuild": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"DscNodeConfigurationCreateOrUpdateParameters" doesn't contain a property called "newNodeConfigurationBuildVersionRequired" but it's present in the example: https://github.com/vrdmr/azure-rest-api-specs/blob/0489a8a2c0bf758e9593a7d21ff507e0899ff698/specification/automation/resource-manager/Microsoft.Automation/stable/2015-10-31/examples/createOrUpdateDscNodeConfiguration.json
"x-ms-enum": { | ||
"name": "JobProvisioningState", | ||
"modelAsString": true | ||
} | ||
}, | ||
"JobCreateParameters": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jianghaolu Its a proxy resource and would not have "Location", "Name" and "Tags".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But in the example I linked above there is - you might want to double check if it's a proxy resource.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll verify and confirm tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed and removed.
@@ -53,14 +53,10 @@ | |||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong example file reference? This operation returns a file for 200.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to discuss this issue. Can we sync offline tomorrow?
}, | ||
{ | ||
"$ref": "./definitions.json#/parameters/ApiVersionParameter" | ||
"$ref": "../../common/v1/definitions.json#/parameters/ApiVersionParameter" | ||
} | ||
], | ||
"responses": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"JobListResult" contains Job, which doesn't have properties "schedule", "jobScheduleId" listed in the example: https://github.com/vrdmr/azure-rest-api-specs/blob/0489a8a2c0bf758e9593a7d21ff507e0899ff698/specification/automation/resource-manager/Microsoft.Automation/stable/2015-10-31/examples/listJobsByAutomationAccount.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've fixed this. But we have a new version of the Jobs resource 2017-05-15-preview
and we have updated the SDK to use the new version as well.
"description": "Gets or sets a Boolean value that indicates true if the parameter is dynamic." | ||
}, | ||
"position": { | ||
"type": "boolean", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is defined as boolean but in the example they are all integers: getActivityInAModulehttps://github.com/vrdmr/azure-rest-api-specs/tree/0489a8a2c0bf758e9593a7d21ff507e0899ff698/specification/automation/resource-manager/Microsoft.Automation/stable/2015-10-31/examples
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
} | ||
], | ||
"responses": { | ||
"200": { | ||
"description": "A single software update configuration.", | ||
"schema": { | ||
"$ref": "./definitions.json#/definitions/softwareUpdateConfiguration" | ||
"$ref": "#/definitions/softwareUpdateConfiguration" | ||
} | ||
}, | ||
"404": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
status codes "404" for operation "SoftwareUpdateConfigurations_GetByName" were present in the swagger spec, however they were not present in x-ms-examples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
@@ -266,7 +266,7 @@ | |||
"200": { | |||
"description": "Return list of software update configurations.", | |||
"schema": { | |||
"$ref": "./definitions.json#/definitions/softwareUpdateConfigurationListResult" | |||
"$ref": "#/definitions/softwareUpdateConfigurationListResult" | |||
} | |||
}, | |||
"404": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
} | ||
], | ||
"responses": { | ||
"200": { | ||
"200": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also add a 200 example response in examples/replaceRunbookDraftContent.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It never generates 200. This was added to fix the linting issues in #2687.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If your service doesn't generate 200 you shouldn't put it in the spec. If the linter reports error then we should fix the linter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed with a suppression at for LongRunningResponseStatusCode
in readme.md
for this.
} | ||
], | ||
"responses": { | ||
"200": { | ||
"200": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above: It never generates 200. This was added to fix the linting issues in #2687.
@@ -443,7 +443,7 @@ | |||
"$ref": "#/definitions/DscConfigurationAssociationProperty", | |||
"description": "Gets or sets the configuration of the node." | |||
}, | |||
"incrementNodeConfigurationBuild": { | |||
"IncrementNodeConfigurationBuild": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the corresponding fields in this example too: https://github.com/vrdmr/azure-rest-api-specs/blob/0489a8a2c0bf758e9593a7d21ff507e0899ff698/specification/automation/resource-manager/Microsoft.Automation/stable/2018-01-15/examples/createOrUpdateDscNodeConfiguration.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
@jianghaolu Could you please take a look at this PR. We have to get the fixes in by Friday. Please let me know if you have any questions. |
@vrdmr Sorry for the delay - Did you get a chance to confirm this? https://github.com/Azure/azure-rest-api-specs/pull/3190/files/0489a8a2c0bf758e9593a7d21ff507e0899ff698#diff-91d25383f8afa4ecf50a11d676f50454 |
There are a few other errors reported in the validator which I'm trying to understand. |
It seems like one error that the model validator is reporting is related to all the |
Automation for azure-sdk-for-javaNothing to generate for azure-sdk-for-java |
@jianghaolu The remaining errors in the Travis model check are looking at |
@@ -247,9 +247,6 @@ | |||
"200": { | |||
"description": "OK" | |||
}, | |||
"404": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused about why we are removing 404 here. If we try to get a schedule, shouldn't we return a 404 if it doesn't exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the specs, we only specify success cases and not the failure cases. If we start specifying error cases, the SDK does not throw an exception and treats it as a normal case.
For more info, @finiteattractor can describe it in more details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Signing off.
where: $.definitions.TestJob.properties | ||
- suppress: DefinitionsPropertiesNamesCamelCase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
signing off on this change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Signing off on suppression changes on camel case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Varad for doing this big refactoring! Changes look good to me.
Thanks @jianghaolu! |
Refactoring Automation Resources to prevent the duplication of Models in different definitions file and creating a single common definition file for definitions/parameters used across Automation.
PR information
api-version
in the path should match theapi-version
in the spec).Quality of Swagger